Fix: Thread-safe DB connection manager with pooling (#212)#252
Fix: Thread-safe DB connection manager with pooling (#212)#252muhammadtihame wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds async database support via SQLAlchemy/asyncpg: new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as FastAPI Route
participant GetDb as get_db()
participant Maker as async_session_maker
participant Pool as Connection Pool
participant Session as AsyncSession
Client->>GetDb: request session
alt Engine initialized
GetDb->>Maker: create session
Maker->>Pool: acquire connection
Pool-->>Maker: provide connection
Maker-->>Session: return AsyncSession
GetDb-->>Client: yield session
Client->>Session: execute queries
alt Success
Client->>GetDb: return normally
GetDb->>Session: close session
Session->>Pool: release connection
else Exception
Client->>GetDb: exception raised
GetDb->>Session: rollback
GetDb->>Session: close session
Session->>Pool: release connection
end
else Engine not initialized
GetDb-->>Client: raise RuntimeError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/database/core.py`:
- Around line 46-57: Remove the redundant explicit close in the async session
context manager: when using the async with async_session_maker() as session
block you should not call await session.close() in the finally; delete that call
so the context manager handles cleanup. Also replace logger.error(...) with
logger.exception(...) in the except block to include the traceback, while
preserving await session.rollback() and re-raising the exception; reference
async_session_maker, session, logger.error -> logger.exception, session.rollback
to locate the changes.
In `@tests/test_db_pool.py`:
- Around line 7-9: The test currently patches app.core.config.settings at module
scope but imports engine and get_db after the patch context, which is fragile
because engine is created at import time and module caching prevents
reinitialization; update tests/test_db_pool.py to apply the patch while
importing/reloading the database module so engine is created with the mocked
configuration—use a pytest fixture that applies patch.dict or patch to set
DATABASE_URL (or patch app.core.config.settings), then
importlib.reload(app.database.core) inside that fixture and yield db_core.engine
and db_core.get_db (or move the import of engine/get_db inside the with
patch(...) block) so engine initialization sees the mocked value.
- Around line 44-45: The async context manager mock is incomplete: instead of
assigning __aexit__.return_value = None make both __aenter__ and __aexit__
awaitable (e.g., replace the current assignments on mock_session with AsyncMock
usage: set mock_session.__aenter__ = AsyncMock(return_value=mock_session) and
mock_session.__aexit__ = AsyncMock(return_value=None)), and apply the same
change to the other async context-manager mock later in the file so both
enter/exit are proper awaitables.
🧹 Nitpick comments (1)
tests/test_db_pool.py (1)
49-52: Unused loop variablesession— rename to_session.Per the static analysis hint, the loop variable is unused within the body. Renaming to
_sessionsignals intentional non-use.- async for session in get_db(): + async for _session in get_db():The same applies to line 80.
|
Fixes #212 |
|
Addressed CodeRabbit feedback:
|
Summary
This PR implements thread-safe database connection management with connection pooling and proper resource cleanup.
Changes
backend/app/database/core.py(async SQLAlchemy engine + session manager)tests/test_db_pool.pyto verify concurrency and rollback behaviordocs/DATABASE_CONNECTION.mddocumentationbackend/app/core/config/settings.pyfor database configurationpyproject.tomlandbackend/requirements.txtto include new dependenciesVerification
Run the following tests: